process: correctly parse Unicode in NODE_OPTIONS by bzoz · Pull Request #34476 · nodejs/node

bzoz · 2020-07-22T11:01:44Z

Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed
correctly.

Fixes: #34399

Reused the code from node_env_var.cc for getting the enviroment variable value.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2020-07-22T11:02:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32470/

nodejs-github-bot · 2020-07-22T14:27:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32475/

lundibundi · 2020-07-22T16:25:16Z

test/parallel/test-unicode-node-options.js

Nit:

Suggested change

const NODE_OPTIONS = `--redirect-warnings=${expected_redirect_value}`;

const result = cp.spawnSync(process.argv0,

['--expose-internals', __filename, 'test'],

{

env: {

NODE_OPTIONS

}

});

const env = { NODE_OPTIONS: `--redirect-warnings=${expected_redirect_value}` };

const result = cp.spawnSync(process.argv0,

['--expose-internals', __filename, 'test'],

{ env });

lundibundi · 2020-07-22T16:25:43Z

test/parallel/test-unicode-node-options.js

Is this necessary?

lundibundi · 2020-07-22T16:37:00Z

test/parallel/test-unicode-node-options.js

Nit: I'd prefer to have explicit check for 'child process' case (i.e. if (process.argv[2] === 'test') {) instead to handle possibly different test start args.

lundibundi · 2020-07-22T16:37:19Z

test/parallel/test-unicode-node-options.js

Suggested change

const result = cp.spawnSync(process.argv0,

const result = cp.spawnSync(process.execPath,

nodejs-github-bot · 2020-08-12T13:47:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32745/

bzoz · 2020-08-12T13:47:22Z

Fixed some nits and rebased.

nodejs-github-bot · 2020-08-13T08:51:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32755/

bzoz · 2020-08-20T00:27:30Z

Ok, this is weird. This fails only on ubuntu1804_sharedlibs_openssl111_x64 with this error message:

ot ok 2466 parallel/test-unicode-node-options
  ---
  duration_ms: 0.144
  severity: fail
  exitcode: 1
  stack: |-
    out/Release/node: /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1: version `OPENSSL_1_1_1b' not found (required by out/Release/node)
    assert.js:103
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    
    1 !== 0
    
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-unicode-node-options.js:18:10)
        at Module._compile (internal/modules/cjs/loader.js:1089:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:1110:10)
        at Module.load (internal/modules/cjs/loader.js:954:32)
        at Function.Module._load (internal/modules/cjs/loader.js:795:14)
        at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
        at internal/main/run_main_module.js:17:47 {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 1,
      expected: 0,
      operator: 'strictEqual'
    }
  ...

It looks like other tests can spawn new Node instances just fine. I'm stuck here 🤔

cjihrig · 2020-08-20T01:26:01Z

test/parallel/test-unicode-node-options.js

Does this help with the current CI failure?

Suggested change

env: { NODE_OPTIONS },

env: { ...process.env, NODE_OPTIONS },

Yes, it helped. Thanks!

Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed correctly. Fixes: nodejs#34399

bzoz · 2020-08-20T01:47:49Z

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/32863/

Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed correctly. Fixes: nodejs#34399 PR-URL: nodejs#34476 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

bzoz · 2020-08-20T10:39:44Z

Green CI, landed in de565ad

Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed correctly. Fixes: #34399 PR-URL: #34476 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Denys Otrishko <shishugi@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 22, 2020

addaleax approved these changes Jul 22, 2020

View reviewed changes

jasnell approved these changes Jul 22, 2020

View reviewed changes

devsnek approved these changes Jul 22, 2020

View reviewed changes

lundibundi approved these changes Jul 22, 2020

View reviewed changes

This was referenced Jul 23, 2020

CI Relability 2020-07-23 JaneaSystems/nodejs-reliability#46

Open

CI Relability 2020-07-24 JaneaSystems/nodejs-reliability#47

Open

bzoz force-pushed the unicode-node-options branch from f5f8a79 to 1a31d81 Compare August 12, 2020 13:46

This was referenced Aug 13, 2020

CI Relability 2020-08-13 JaneaSystems/nodejs-reliability#67

Open

CI Reliability 2020-08-13 nodejs/reliability#42

Open

cjihrig reviewed Aug 20, 2020

View reviewed changes

bzoz added 5 commits August 20, 2020 03:42

process: correctly parse Unicode in NODE_OPTIONS

12d0405

Fixes an issue on Windows, where Unicode in NODE_OPTIONS was not parsed correctly. Fixes: nodejs#34399

fixup: linter

7b15fa7

fixup: nits

c88bb39

fixup: add logging

aa20c6f

fixup: also pass env

15ac48a

bzoz force-pushed the unicode-node-options branch from 398d7d5 to 15ac48a Compare August 20, 2020 01:46

bzoz closed this Aug 20, 2020

danielleadams mentioned this pull request Aug 20, 2020

V14.9.0 proposal #34852

Merged

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

-  const NODE_OPTIONS = `--redirect-warnings=${expected_redirect_value}`;
-  const result = cp.spawnSync(process.argv0,
-                              ['--expose-internals', __filename, 'test'],
-                              {
-                                env: {
-                                  NODE_OPTIONS
-                                }
-                              });
+  const env = { NODE_OPTIONS: `--redirect-warnings=${expected_redirect_value}` };
+  const result = cp.spawnSync(process.argv0,
+                              ['--expose-internals', __filename, 'test'],
+                              { env });

	const result = cp.spawnSync(process.argv0,
	const result = cp.spawnSync(process.execPath,

	env: { NODE_OPTIONS },
	env: { ...process.env, NODE_OPTIONS },

Uh oh!

Conversation

bzoz commented Jul 22, 2020

Checklist

Uh oh!

nodejs-github-bot commented Jul 22, 2020

Uh oh!

nodejs-github-bot commented Jul 22, 2020

Uh oh!

lundibundi Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

lundibundi Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

lundibundi Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

lundibundi Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 12, 2020

Uh oh!

bzoz commented Aug 12, 2020

Uh oh!

nodejs-github-bot commented Aug 13, 2020

Uh oh!

bzoz commented Aug 20, 2020

Uh oh!

cjihrig Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

bzoz Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

bzoz commented Aug 20, 2020

Uh oh!

bzoz commented Aug 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants